-
-
Notifications
You must be signed in to change notification settings - Fork 385
Glasgow | Jan-26 | Elisabeth Matulian | Sprint 1 | Wireframe #939
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Glasgow | Jan-26 | Elisabeth Matulian | Sprint 1 | Wireframe #939
Conversation
This comment has been minimized.
This comment has been minimized.
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
jenny-alexander
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Elisabeth-Matulian 👋🏻 - you've added some good content to your wireframe exercise! I left a few suggestions for you to make in order to enhance your code submission.
Wireframe/index.html
Outdated
| more</a> | ||
| </article> | ||
| <article> | ||
| <img src="placeholder.svg" alt="" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an image related to the article topic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
Wireframe/index.html
Outdated
| <a href="https://www.productplan.com/glossary/wireframe/">Read more</a> | ||
| </article> | ||
| <article> | ||
| <img src="placeholder.svg" alt="" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an image related to the article topic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
Wireframe/index.html
Outdated
| </header> | ||
| <main> | ||
| <article> | ||
| <img src="placeholder.svg" alt="" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an image related to the article topic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
Wireframe/index.html
Outdated
| </main> | ||
| <footer> | ||
| <p> | ||
| This is the default, provided code and no changes have been made yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 Perhaps you could add a little bit of information to the footer. This could be:
- contact email address or link
- a few social links to CYF
- a copyright notice like '© 2026 [Name]'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your review. Could you check the changes I made? :)
jenny-alexander
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙂 The wireframe is looking much better now - thanks for making those changes.
You have made good use of the alt attribute within the <image> tag to provide meaningful text to screen readers.
Here are some areas to improve:
- I see a few styling issues:
- the article titles do not quite line up
- the Read more buttons are not the same size.
- Files in PR:
- Addition of files:
- Are any of the files within the
/wireframe examples_filesdirectory used? If not, the entire directory should be removed from the PR. - Is the
wireframe examples.htmlfile used? If not, it should be removed.
- Are any of the files within the
- Deletion of file:
- The
placeholder.svgfile is a part of this exercise and it should not be deleted by your PR. Can you please undo this?
- The
- Addition of files:
| <a href="https://git-scm.com/book/en/v2/Git-Branching-Branches-in-a-Nutshell">Read more</a> | ||
| </article> | ||
| </main> | ||
| <footer> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your footer is looking better now. However, I see a few issues that should be fixed.
- Since you are using
<li>elements, the default will show a bullet point. Can you find a way to ensure that those bullet points are not displayed?
- The exercise instructions say: "The page footer is fixed to the bottom of the viewport." Right now, when I scroll up and down on your webpage, the footer is not fixed. If you aren't sure what a fixed footer is, here is a good example from W3Schools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your helpful feedback. I’ve made changes but I'm not sure if I correctly understand the way the footer should look.
I’d really appreciate it if you could take another look and let me know if everything looks good now.
Thanks again for your guidance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Elisabeth-Matulian - I had a look at your footer and I think there are a few opportunities to improve it.
I took a video recording for you to see what I mean. I also added an orange background colour to it. You can see that your footer isn't spanning the entire width of the screen.
Can you make changes to:
1 - Add a background color so the user can always see your footer?
2 - Ensure that the footer takes up the entire width of the page?
Screen.Recording.2026-01-20.at.3.07.46.PM.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for taking the time to review my work and for the detailed feedback.
I’ve now updated the footer to add a background colour and ensured it spans the full width of the page.
When you have a moment, could you please have another look and let me know if everything looks correct now?
Thanks again for your help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Elisabeth-Matulian I'm glad you found my comments useful!
I reviewed the footer and see it's now visible ✅ and it's full width ✅. I ran an accessibility report against your page and see a small issue with the contrast ratio of the footer. Can you take a look?
Also, wanted to point out that the 'Read more' buttons on the wireframe have a complete border around them and yours only have an underline.
https://github.com/CodeYourFuture/Module-Onboarding/tree/main/Wireframe
At this point, I'm not going to ask you change that -> but wanted to highlight to ensure in the future that you try to follow the wireframe as closely as possible!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for reviewing my updates. I’m glad the previous changes improved the footer.
I’ve noted the accessibility contrast issue and adjusted it. When you have a moment, could you please take another look at my full submission?
…ed files, added back placeholder.svg
take up the entire width of the page
|
Nicely done @Elisabeth-Matulian! |

Learners, PR Template
Self checklist
Changelist
I arranged the blocks according to the provided template. I fixed the footer at the bottom of the page and filled the blocks with articles.
Questions
Needs review